Skip to content

Force-close channels on reorg only if the funding is unconfirmed #1461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

Currently, if a channel's funding is locked in and then later
reorg'd back to half of the channel's minimum-depth we will
immediately force-close the channel. However, this can happen at
the fork-point while processing a reorg, and generally reorgs do
not reduce the block height at all, making this a rather useless
endeavor.

Ideally we'd never auto-force-close channels at all due to a reorg,
instead simply marking it as inactive until the funding
transaction is re-confirmed (or allowing the user to attempt to
force-close or force-closing once we're confident we have
completed reorg processing if we're at risk of losing funds
already received in the channel).

Sadly, we currently do not support changing a channel's SCID and
updating our SCID maps, so we cannot yet remove the automated
force-close logic. Still, there is no reason to do it until a
funding transaction has been removed from the chain.

This implements that change - only force-closeing once a channel's
funding transaction has been reorg'd out (still potentially at a
reorg's fork point). This continues to imply a 1-confirmation
channel will always be force-closed after a reorg of the funding
transaction, and will imply a similar behavior with 0-conf
channels.

Currently, if a channel's funding is locked in and then later
reorg'd back to half of the channel's minimum-depth we will
immediately force-close the channel. However, this can happen at
the fork-point while processing a reorg, and generally reorgs do
not reduce the block height at all, making this a rather useless
endeavor.

Ideally we'd never auto-force-close channels at all due to a reorg,
instead simply marking it as inactive until the funding
transaction is re-confirmed (or allowing the user to attempt to
force-close or force-closing once we're confident we have
completed reorg processing if we're at risk of losing funds
already received in the channel).

Sadly, we currently do not support changing a channel's SCID and
updating our SCID maps, so we cannot yet remove the automated
force-close logic. Still, there is no reason to do it until a
funding transaction has been removed from the chain.

This implements that change - only force-closeing once a channel's
funding transaction has been reorg'd out (still potentially at a
reorg's fork point). This continues to imply a 1-confirmation
channel will always be force-closed after a reorg of the funding
transaction, and will imply a similar behavior with 0-conf
channels.
@TheBlueMatt TheBlueMatt mentioned this pull request May 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2022

Codecov Report

Merging #1461 (1568097) into main (9bdce47) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1461      +/-   ##
==========================================
+ Coverage   90.90%   91.13%   +0.23%     
==========================================
  Files          75       75              
  Lines       41616    43390    +1774     
  Branches    41616    43390    +1774     
==========================================
+ Hits        37829    39544    +1715     
- Misses       3787     3846      +59     
Impacted Files Coverage Δ
lightning/src/ln/channel.rs 88.38% <100.00%> (ø)
lightning/src/ln/reorg_tests.rs 100.00% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 97.09% <0.00%> (-0.08%) ⬇️
lightning/src/routing/scoring.rs 95.07% <0.00%> (+0.70%) ⬆️
lightning/src/ln/channelmanager.rs 88.05% <0.00%> (+3.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bdce47...1568097. Read the comment docs.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, maybe can be a good introduce atest to cover this edge case?

@TheBlueMatt TheBlueMatt force-pushed the 2022-05-unconf-0-not-half branch from 61f097e to 6e5169b Compare May 2, 2022 15:21
vincenzopalazzo
vincenzopalazzo previously approved these changes May 3, 2022
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TheBlueMatt
Copy link
Collaborator Author

Actually, partially unded that last commit - if we're doing FullBlockViaListen it still makes sense but we may also be jumping backwards, which we want to do directly and not go backwards 5 blocks and then one block, which would end up being pretty similar to FullBlockViaListen (which goes back one block at a time).

@TheBlueMatt
Copy link
Collaborator Author

Squashed the last fixup commit

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cr-ACK 1568097

@TheBlueMatt TheBlueMatt added this to the 0.0.107 milestone May 3, 2022
@valentinewallace valentinewallace merged commit 72e9f53 into lightningdevkit:main May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants